Cm/ol crowding accuracy logging #1862
Conversation
Coverage of commit
|
…eens into cm/ol-crowding-accuracy-logging
Coverage of commit
|
There was a problem hiding this comment.
Just a handful of things, mostly sanity checking.
The only thing I'm really concerned about is the lack of :restart opt on the start_child call. Assuming I interpreted the docs correctly (I definitely might not have!), the default :restart behavior will cause the supervisor to indefinitely restart any genserver that it spawns, even if the genserver shuts itself down with {:stop, :shutdown, state}. Which would cause an ever-increasing number of processes! 😱
lib/screens/vehicles/parser.ex
Outdated
| defp parse_carriage_occupancy_status("NO_DATA_AVAILABLE"), do: :no_data | ||
| defp parse_carriage_occupancy_status("MANY_SEATS_AVAILABLE"), do: :not_crowded | ||
| defp parse_carriage_occupancy_status("FEW_SEATS_AVAILABLE"), do: :not_crowded | ||
| defp parse_carriage_occupancy_status("STANDING_ROOM_ONLY"), do: :some_crowding | ||
| defp parse_carriage_occupancy_status("CRUSHED_STANDING_ROOM_ONLY"), do: :crowded | ||
| defp parse_carriage_occupancy_status("FULL"), do: :crowded | ||
| defp parse_carriage_occupancy_status("NOT_ACCEPTING_PASSENGERS"), do: :closed | ||
| defp parse_carriage_occupancy_status(_), do: nil |
There was a problem hiding this comment.
It's a little unusual to push this app-specific business logic all the way down to the parser, but I guess as long as we're sure we want to do this mapping from the v3 API's classification groups to our own in all cases, it's better to put here for consistency.
Could you also update the struct typespec in Screens.Vehicles.Vehicle to reflect that the carriage-level occupancy status values are now different from the vehicle-level occupancy statuses?
(This also makes me wonder if we should make this same change to parse_occupancy_status, for consistency. It feels weird to keep the v3 API's classifications for one but not the other.)
There was a problem hiding this comment.
This may have been too impulsive of a change, cause you're right. This does feel weird. Would it be better to add this conversion as a util function instead?
There was a problem hiding this comment.
A util function could be good, yeah! Reusable but also doesn't force this mapping from v3 API classification to our classification in all cases.
Coverage of commit
|
Coverage of commit
|
Asana task: Add logging to triptych app - pt 1 1/2
During the heuristics discussion, we realized we aren't able to capture crowding widget accuracy while the widget is on screen. In this PR, I added a
DynamicSupervisorthat is able to spin up background processes that log crowding data every 5 seconds until the train is stopped at the current station. With these logs, we will be able to compare the existing logs ([train_crowding car_crowding_info]) with the added logs ([train_crowding car_crowding_accuracy_info]). If thecar_crowding_levelschange at all, we know that we were not able to give accurate crowding data to riders before the train arrived.I did make one change to the widget logic. I moved the crowding class translations up to the vehicle parser. This allows us to see if a crowding class change results in a color change on screen.